Decouple repository root resolver from internal logic #2957
Conversation
… and update options in rally and stream runners
…factor tests and added readme note
|
@mrodm i've updated the code with your feedback. i usually mark resolved the comments that have been addressed, please re-open if you find anything pending. thanks |
There was a problem hiding this comment.
It looks great, it seems to solve links resolution and does a good refactor to allow injection of the path to the root repository.
I did some tests and found some issues, commented in the files, though maybe some of them can be solved in follow ups.
Many comments are nitpicking, no need to do anything about them if you prefer current code.
There was a problem hiding this comment.
I have tried to link a fields file that includes fields with external: ecs and elastic-package fails to resolve these fields when building the package, with errors like:
Error: building package failed: invalid content found in built zip package: found 7 validation errors:
1. file ".../github.com/elastic/integrations/build/packages/system-2.6.1.zip/data_stream/cpu/fields/agent.yml" is invalid: field cloud.account.id with external key defined ("ecs") but no _dev/build/build.yml found
I guess it is trying to read the build.yml file from the target directory instead of from the source directory. It only happens with linked files.
Not sure if the issue is introduced with the current PR, so we can handle this in a follow up, as you prefer.
There was a problem hiding this comment.
could you walk me through this test? i am not sure i understood correctly the operation here.
Create a .link file for ecs.yml of one package ?
There was a problem hiding this comment.
This can be reproduced with the following commands, from the integrations repository:
mkdir packages/system/_dev/shared
mv packages/system/data_stream/cpu/fields/agent.yml packages/system/_dev/shared/agent.yml
echo ../../../_dev/shared/agent.yml > packages/system/data_stream/cpu/fields/agent.yml.link
elastic-package links update
elastic-package build -C packages/systemThere was a problem hiding this comment.
I've been able to reproduce this. The cause i've found is that, when the build command is resolving the external references, the link files are not yet at the build directory of the package. The linked files are added to the build bundle after, so when validation is run, the file for agent.yml has not been transformed/resolved and is the original one.
I've changed the order of the steps to solve this. So now links are resolved, and so, the linked files are correctly copied into the bundle. After this, the external references are resolved correctly.
…use value receiver instead of pointer
… handling in tests
…th to PackageRootPath
…and update related tests
…pdate related tests
…esolve external fields
💔 Build Failed
Failed CI Steps
History
|
4 participants
Resolves #2797
Summary
Refactored the linked files system to use explicit
os.Rootparameters for secure repository access. This change fixes a bug where linked files were copied to incorrect directories and reveals a secondary issue where external fields were resolved before linked files were included in the build bundle.Problems Fixed
Linked files copied to wrong directory: The
workDirinLinksFSwas treated inconsistently (absolute vs relative), causing incorrect path resolution. Now standardized to always be relative to repository root.External fields resolved too early: The build process resolved external fields before copying linked files, causing missing field definitions. Reordered build steps to copy linked files first.
Key Changes
Repository Root Handling:
CreateLinksFSFromPath()now requires explicit*os.Rootparameterdefer repositoryRoot.Close()API Changes:
FindPackageRoot()signature:(string, bool, error)→(string, error)ErrPackageRootNotFoundsentinel errorBuildOptions.PackageRootrenamed toPackageRootPathand addedRepositoryRootfieldOptions.RootPathrenamed toPackageRootPathand addedRepositoryRootfieldBuild Process:
ELASTIC_PACKAGE_REPOSITORY_LICENSEnow expects paths relative to repository rootBreaking Changes
Functions that previously auto-discovered repository root now require it as a parameter:
CreateLinksFSFromPath(repositoryRoot, workDir)BuildPackage()requiresRepositoryRootin optionsNewForPackage()requiresRepositoryRootin optionsTesting